Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

566 recursive instantiate #989

Merged
merged 2 commits into from
Sep 28, 2020
Merged

566 recursive instantiate #989

merged 2 commits into from
Sep 28, 2020

Conversation

omry
Copy link
Collaborator

@omry omry commented Sep 19, 2020

  • Implement recursive instantiation
  • Clean ObjectConf and params support deprecated in Hydra 1.0.
  • Change API to default to recursive
  • Update documentation to reflect changes
    • Explain param overrides and how it works with nested instantiations.
    • Add examples
    • Add an upgrade page explaining change for default recursive (no need for an upgrade page).

Closes #566
Closes #995

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 19, 2020
@omry omry force-pushed the 566-recursive-instantiate branch 4 times, most recently from d2be456 to 5bd94d5 Compare September 21, 2020 08:54
@omry omry linked an issue Sep 21, 2020 that may be closed by this pull request
@omry
Copy link
Collaborator Author

omry commented Sep 21, 2020

#995 : documentation followup

@omry omry force-pushed the 566-recursive-instantiate branch 11 times, most recently from 67eade6 to dce15c0 Compare September 24, 2020 23:42
@lgtm-com
Copy link
Contributor

lgtm-com bot commented Sep 25, 2020

This pull request introduces 2 alerts when merging b9eaf50 into a523de2 - view on LGTM.com

new alerts:

  • 1 for Unreachable code
  • 1 for Variable defined multiple times

@omry omry force-pushed the 566-recursive-instantiate branch from b9eaf50 to 6ab3129 Compare September 25, 2020 01:18
@lgtm-com
Copy link
Contributor

lgtm-com bot commented Sep 25, 2020

This pull request introduces 2 alerts when merging 6ab3129 into 108af1a - view on LGTM.com

new alerts:

  • 1 for Unreachable code
  • 1 for Variable defined multiple times

@omry omry force-pushed the 566-recursive-instantiate branch from 6ab3129 to 8343d38 Compare September 25, 2020 02:01
@CatOfTheCannals
Copy link

Hi!
Will this new feature be released to a 1.0.4 or to a 1.1.0 version?
Will it be available soon?

@omry
Copy link
Collaborator Author

omry commented Sep 25, 2020

Hi @CatOfTheCannals,
As you can tell by looking at the diff size, it's pretty big.
It will be released with Hydra 1.1, it will be available in master and later in dev or rc releases before Hydra 1.1 gets released.

@omry omry force-pushed the 566-recursive-instantiate branch from e4ac92b to 011b6cd Compare September 25, 2020 22:51
Comment on lines 64 to 65
cls = _get_cls_name(config)
type_or_callable = _locate(cls)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for supporting objects! Since now any object can be put in the dict, can you allow _target_ to be the callable object itself?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is potentially doable but out of scope for this diff.
Can you open a new feature request and show an example where this would be preferred over passing the class/function name?

@omry omry force-pushed the 566-recursive-instantiate branch from f0ec938 to 6ac5a62 Compare September 26, 2020 01:06
@omry omry merged commit f386932 into master Sep 28, 2020
@omry omry deleted the 566-recursive-instantiate branch September 28, 2020 19:18
@alexcoca
Copy link

alexcoca commented Jun 28, 2022

@omry I don't know if I should open a feature request or if this is already possible, but suppose you have a complex hierarchy where most objects have to be recursively instantiated but for some we wish to defer the instantiation so that it can be managed inside the program. For example, if we wish to run large models sequentially on the same GPU, we wish to instantiate the second once we are done with the first. I had a feeling that _recursive_ special key could be used to achieve this so tried this configuration

bleurt:
    _recursive_: false
    _target_: datasets.load_metric

but I'm getting the error

Error in call to target 'datasets.load.load_metric':
TypeError("load_metric() missing 1 required positional argument: 'path'")
full_key: architecture.paraphrase_decoder.semantic_faithfulness_model

Set the environment variable HYDRA_FULL_ERROR=1 for a complete stack trace```

which suggests that hydra does not care about _recursive_ . I definitely can't pass _recursive_ false to the the instantiation call for the entire object because the structure is complex and it would take a huge refactor to achieve this. Is this feature not supported?

@Jasha10
Copy link
Collaborator

Jasha10 commented Jun 28, 2022

@alexcoca I think you will need to pass _recursive_: false to the config that is one level above bleurt:

foo:
  _target_: bar
  _recursive_: false
  bleurt:
    _target_: datasets.load_metric

With this setup, I believe foo will be instantiated and bleurt will not.

@alexcoca
Copy link

@Jasha10 Yes, this was the only way to get things working. I removed bleurt and created a dummy function that returned datasets.load_metric as a work around as, bleurt was sadly at the same of the hierarchy with a bunch of objects that did need to be instantiated so I could not have _recursive_ at the level above. I'm happy enough with this workaround ... Having looked through the source code I understand that the logic is to avoid instantiating children of nodes where _recursive_: false is specified. Obviously, this would require that in my case the parent of bleurt specified _recusive_: false, as you suggest. Thanks for your help, I learned a lot.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Document recursive instantiation once API stabilizes Recursive instantiation
6 participants